Skip to content

Conversation

@patrickfreed
Copy link
Contributor

This PR runs SwiftFormat on the driver and adds it as part of our linting task.

In general, I tried to emulate our swiftlint config as much as possible so that this formatter would help us enforce existing rules rather than making up new ones. The one notable exception to that is the wrapArguments rule, which I've standardized to "before-first". I don't think there is a right answer here as to which option is better, but I do think that selecting any option for consistency is the right idea (as @Utagai said long ago). The majority of the diff from this PR is due to this rule.

e.g.

public func hello(a: Int, 
                  b: Int) throws -> Int { ... }

let a = try hello(a: 1,
                  b: 2)

becomes

public init(
    a: Int,
    b: Int
) throws { ... }

let a = try hello(
    a: 1,
    b: 2
)

In general, I found that this approach to wrapping produces far more readable and condensed code than only doing the "after-first" option, so I went with it.

Another thing to note is that this rule only applies to how we format multiline arguments; it doesn't decide when we do it.

e.g. the following will be untouched by the formatter:

public func a(a: Int, b: Int) throws -> Int { ... }

let b = a(a: 1, b: 2)

This means that not all output produced by the formatter will pass swiftlint due to line length issues, so manual line breakage may be necessary. The formatter will make sure that the way the lines are broken is fixed up, though.

Why not swift-format?
There is an official formatter being worked on by Apple called swift-format. I decided not to use it for this because it supports a very small number of rules compared to SwiftFormat. I think that long term, it may make sense to start using the official formatter once it becomes more mature.

expect(try encoder.encode(Numbers(uint64: 4294967295))).to(equal(["uint64": .int64(4294967295)]))
expect(try encoder.encode(Numbers(uint: 4294967295))).to(equal(["uint": .int64(4294967295)]))
expect(try encoder.encode(Numbers(uint64: 4_294_967_295))).to(equal(["uint64": .int64(4_294_967_295)]))
expect(try encoder.encode(Numbers(uint: 4_294_967_295))).to(equal(["uint": .int64(4_294_967_295)]))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding underscores in large number literals is another interesting change that's on by default. Let me know what you guys think about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 👍

@patrickfreed patrickfreed marked this pull request as ready for review October 18, 2019 23:24
Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally lgtm.

in general, I found that this approach to wrapping produces far more readable and condensed code than only doing the "after-first" option
what does after first look like?

--wraparguments "before-first"

# if x == 1 vs if 1 == x
--disable yodaConditions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one seems reasonable to leave on to me, 1 == x looks kinda weird

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yodaConditions actually enforces the 1 == x, so it's being disabled here; I agree, look weird it does. It's called a "yoda" condition because if read it sounds like you're speaking like Yoda from star wars...lol

Copy link
Contributor

@kmahar kmahar Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lmao well... good for star wars fans I suppose

Copy link
Contributor Author

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first code snippets in the PR description are examples of the "after-first" way. "after-first" and "before-first" mean "line break before/after first parameter"

--wraparguments "before-first"

# if x == 1 vs if 1 == x
--disable yodaConditions
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yodaConditions actually enforces the 1 == x, so it's being disabled here; I agree, look weird it does. It's called a "yoda" condition because if read it sounds like you're speaking like Yoda from star wars...lol

@patrickfreed patrickfreed merged commit 8c9a4b4 into master Oct 22, 2019
@patrickfreed patrickfreed deleted the formatter branch October 24, 2019 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants